Skip to content

add override capabilities to repeated capabilities documentation - #2183

Open
elebel-emerson wants to merge 20 commits into
ni:masterfrom
elebel-emerson:updateRepeatedCaps
Open

add override capabilities to repeated capabilities documentation#2183
elebel-emerson wants to merge 20 commits into
ni:masterfrom
elebel-emerson:updateRepeatedCaps

Conversation

@elebel-emerson

@elebel-emerson elebel-emerson commented Jul 2, 2026

Copy link
Copy Markdown

- [ ] I've updated CHANGELOG.md if applicable.

  • I've added tests applicable for this pull request

What does this Pull Request accomplish?

Change generated Repeated Capabilities documentation to support metadata-defined descriptions, examples, and identifier guidance for capability-specific behavior.

What testing has been done?

Manually testing change in config.py propagates to html documentation
Create a unit test

@codecov-commenter

codecov-commenter commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.10%. Comparing base (1e272b6) to head (0bec19d).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2183   +/-   ##
=======================================
  Coverage   88.09%   88.10%           
=======================================
  Files          88       88           
  Lines       22756    22765    +9     
=======================================
+ Hits        20047    20056    +9     
  Misses       2709     2709           
Flag Coverage Δ
codegenunittests 84.98% <100.00%> (+0.08%) ⬆️
nidcpowersystemtests 94.34% <ø> (ø)
nidcpowerunittests 89.53% <ø> (ø)
nidigitalsystemtests 92.02% <ø> (ø)
nidigitalunittests 68.44% <ø> (ø)
nidmmsystemtests 92.29% <ø> (ø)
nifakeunittests 86.01% <ø> (ø)
nifgensystemtests 94.32% <ø> (ø)
nimodinstsystemtests 73.85% <ø> (ø)
nimodinstunittests 94.20% <ø> (ø)
nirfsasystemtests 82.66% <ø> (ø)
nirfsaunittests 63.55% <ø> (ø)
nirfsgsystemtests 81.12% <ø> (ø)
niscopesystemtests 89.96% <ø> (ø)
niscopeunittests 43.20% <ø> (ø)
nisesystemtests 91.50% <ø> (ø)
niswitchsystemtests 81.56% <ø> (ø)
nitclksystemtests 94.87% <ø> (ø)
nitclkunittests 98.26% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@elebel-emerson
elebel-emerson marked this pull request as ready for review July 2, 2026 19:21
@ni-jfitzger

Copy link
Copy Markdown
Collaborator

Why are you changing the file modes of our metadata files?

@ni-jfitzger

Copy link
Copy Markdown
Collaborator

Note: if you pull the latest changes, the system tests will pass.
Not really a requirement for this change, but the PR Checks would look better.

@elebel-emerson

elebel-emerson commented Jul 22, 2026

Copy link
Copy Markdown
Author

Why are you changing the file modes of our metadata files?

I did not explicitly touch those files, doing tox -e codegen touched those files, and I just committed.
Removed the changed to file mode in 92b5d19

Comment thread build/templates/rep_caps.rst.mako Outdated
@elebel-emerson
elebel-emerson marked this pull request as draft July 30, 2026 19:08
@elebel-emerson
elebel-emerson marked this pull request as ready for review July 30, 2026 19:38
@elebel-emerson

Copy link
Copy Markdown
Author

Note: if you pull the latest changes, the system tests will pass. Not really a requirement for this change, but the PR Checks would look better.

git pull origin says I am up to date with origin. Still one failing change that does not seem correlated to this change

@ni-jfitzger

Copy link
Copy Markdown
Collaborator

Note: if you pull the latest changes, the system tests will pass. Not really a requirement for this change, but the PR Checks would look better.

git pull origin says I am up to date with origin. Still one failing change that does not seem correlated to this change

Yeah, any system test failures are unrelated to your changes. You haven't touched the API or the System Tests.

@marcoskirsch

Copy link
Copy Markdown
Member

"overwrite" should be "override" (nim-python terminology)

@marcoskirsch

Copy link
Copy Markdown
Member

You are missing a NI-FAKE example

Comment thread build/templates/rep_caps.rst.mako Outdated
@elebel-emerson elebel-emerson changed the title add overwrite to repeated capabilities add override capabilities to repeated capabilities documentation Aug 6, 2026
@elebel-emerson

Copy link
Copy Markdown
Author

"overwrite" should be "override" (nim-python terminology)

Hopefully changed all instances of "overwrite" to "override"

You are missing a NI-FAKE example

Added unit tests in test_rep_caps_template.py

@ni-jfitzger

Copy link
Copy Markdown
Collaborator

Your changes to metadata_add_all.py are causing related unit tests to fail.

@elebel-emerson

Copy link
Copy Markdown
Author

Your changes to metadata_add_all.py are causing related unit tests to fail.

Should have fixed failures with Travic CI

@ni-jfitzger

Copy link
Copy Markdown
Collaborator

Why are you making a functional change?
I thought we talked about this and determined that chained rep caps already provided the necessary functionality.

Comment thread docs/nidcpower/rep_caps.rst
@elebel-emerson

Copy link
Copy Markdown
Author

Note: if you pull the latest changes, the system tests will pass. Not really a requirement for this change, but the PR Checks would look better.

git pull origin says I am up to date with origin. Still one failing change that does not seem correlated to this change

Yeah, any system test failures are unrelated to your changes. You haven't touched the API or the System Tests.

Pending some system_tests but everything should be up to date with main, let's hope the system tests do not fail again

Comment thread build/templates/rep_caps.rst.mako Outdated
Comment thread build/templates/rep_caps.rst.mako Outdated
Comment thread build/templates/rep_caps.rst.mako Outdated
Comment thread build/templates/rep_caps.rst.mako Outdated
Comment thread docs/nifgen/rep_caps.rst Outdated
@ni-jfitzger

Copy link
Copy Markdown
Collaborator

You should pull upstream/master.
nirfsa has documentation that isn't present in your branch, so you may introducing changes to it that we can't see.

@ni-jfitzger

Copy link
Copy Markdown
Collaborator

If you see any changes in the existing rep_caps.rst, that's probably not a good sign.
The goal here is to add new support without affecting existing documentation, right?

Comment thread src/nifake/metadata/config.py Outdated
@elebel-emerson

Copy link
Copy Markdown
Author

If you see any changes in the existing rep_caps.rst, that's probably not a good sign. The goal here is to add new support without affecting existing documentation, right?

Changed to remove all changes to the generated .rst files

@elebel-emerson

Copy link
Copy Markdown
Author

You should pull upstream/master. nirfsa has documentation that isn't present in your branch, so you may introducing changes to it that we can't see.

Should be up to date from main, pending pipeline tests

Comment thread build/helper/metadata_add_all.py
Comment thread build/templates/rep_caps.rst.mako Outdated
Comment thread build/unit_tests/test_rep_caps_template.py Outdated
Comment thread src/nifake/metadata/config.py Outdated
Comment thread build/unit_tests/test_metadata_add_all.py Outdated
Comment on lines +54 to +57
${example.split('\n\n', 1)[0].replace('\n', '\n ')}

passes a string of :python:`'${prefix}0, ${prefix}1, ${prefix}2'` to the set attribute function.
% if '\n\n' in example:
${textwrap.indent(example.split('\n\n', 1)[1], ' ')}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, but I think it would be less brittle if you did

{
  "examples": [{
    "code": "",
    "description": ""},
   { ... },
  ]
}

or something like that.

That way you wouldn't have to mess with the newlines and we could handle multiline descriptions. Will a single line description get wrapped, anyway? Yes, it should be. But they're less clean and linting tools might not like long lines (I don't know).

Comment on lines +1092 to +1096
# actual_config = copy.deepcopy(config_input)
actual_config = copy.deepcopy(config_with_custom_rep_cap_documentation)
actual_config['use_locking'] = False
expected = copy.deepcopy(config_expected)
expected = copy.deepcopy(config_with_custom_rep_cap_documentation_expected)
# expected = copy.deepcopy(config_expected)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having these commented out lines will just lead to confusion.

assert "session.resources['dev0/res1'].channel_enabled = True" in rendered

# Generic auto-prefix guidance should be suppressed when override disables it.
# Custom documentation should overwrite the generic auto-prefix guidance.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Custom documentation should overwrite the generic auto-prefix guidance.
# Custom documentation should override the generic auto-prefix guidance.

@ni-jfitzger ni-jfitzger left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is okay, but some of the template code feels a bit brittle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants